Skip to content

Enhance indentation of blocks - #151

Merged
immerrr merged 4 commits into
immerrr:masterfrom
edam:indent-nested-blocks
May 8, 2020
Merged

immerrr merged 4 commits into
immerrr:masterfrom
edam:indent-nested-blocks

Conversation

@edam

@edam edam commented Oct 2, 2019

Copy link
Copy Markdown
Contributor

Added lua-indent-nested-block-content-align var, which disables the aligning of content of nested blocks to the opening brace and indents normally. It defaults to t, which is the previous behaviour.

E.g. 1:

With lua-indent-nested-block-content-align set t (default), then:

call_some_fn( something, {
                val = 5,
                another = 6,
} )

Wth lua-indent-nested-block-content-align set nil, then:

call_some_fn( something, {
    val = 5,
    another = 6,
} )

E.g. 2:

With lua-indent-nested-block-content-align set t (default), then:

local def = {
  some_very_long_name = { fn =
                            function()
                              return true
                            end
  }
}

Wth lua-indent-nested-block-content-align set nil, then:

local def = {
  some_very_long_name = { fn =
      function()
        return true
      end
  }
}

@immerrr

immerrr commented Oct 2, 2019

Copy link
Copy Markdown
Owner

Hey, thank you for the contribution!

It has docs, maintains backward compability, and the change is very focused and self-contained. Great stuff! Could you please add some tests to ensure this behaviour is not lost in any potential refactoring down the road?

@kidd kidd mentioned this pull request Oct 14, 2019
@edam
edam force-pushed the indent-nested-blocks branch from 1068951 to aee46da Compare November 18, 2019 20:37
@edam edam changed the title added lua-indent-nested-block-content-align var Enhance indentation of blocks Nov 18, 2019
@edam

edam commented Nov 18, 2019

Copy link
Copy Markdown
Contributor Author

I've also added lua-indent-close-paren-align var, to change the way the close parenthesis are aligned, so that they always align with the beginning of the line with the open parenthesis on. It defaults to t, which is the previous behaviour.

E.g.,

With lua-indent-close-paren-align set to t (the default), then:

local foo = setmetatable( {
    a = 4,
    b = 5,
                          }, {
    __index = some_func,
} )

With lua-indent-close-paren-align set to nil, then:

local foo = setmetatable( {
    a = 4,
    b = 5,
}, {
    __index = some_func,
} )

@immerrr

immerrr commented Nov 19, 2019

Copy link
Copy Markdown
Owner

Hey, awesome stuff with the tests, but I'm not sure if we need the close-paren variable: if you are not aligning the block, there is not much sense in aligning the closing brace.

@edam

edam commented Nov 26, 2019

Copy link
Copy Markdown
Contributor Author

The nested-block-content-align variable only affects the content of nested blocks. The lua-indent-close-paren-align variable does something different.

E.g.,

Default settings:

call_some_fn( something, {
                val = 5,
                val = 7,
                         }, {
                val = 6,
} )

With lua-indent-nested-block-content-align set to nil:

call_some_fn( something, {
    val = 5,
    val = 7,
                         }, {
    val = 6,
} )

With lua-indent-close-paren-align set to nil:

call_some_fn( something, {
                val = 5,
                val = 7,
}, {
                val = 6,
} )

With both lua-indent-nested-block-content-align and lua-indent-close-paren-align set to nil:

call_some_fn( something, {
    val = 5,
    val = 7,
}, {
    val = 6,
} )

@edam

edam commented Nov 26, 2019

Copy link
Copy Markdown
Contributor Author

Would you like me to combine lua-indent-nested-block-content-align and lua-indent-close-paren-align in to a single variable?

@immerrr

immerrr commented Nov 27, 2019 •

Copy link
Copy Markdown
Owner

Would you like me to combine lua-indent-nested-block-content-align and lua-indent-close-paren-align in to a single variable?

Hmm... I think I missed one case here.

My original idea stemmed from the fact that there could be different preferences w.r.t. where should the table content go:

  • either aligned to the opening brace plus indented (current behaviour in lua-mode)
  • or indented as the line of the opening brace plus one offset (the most popular suggested alternative)

But I couldn't find a reason for the closing brace to be anywhere else but at content indentation minus 1 indentation offset. This was about the case when there is no potential alignment inside the table, i.e. when the inside table elements start on the subsequent line,

foo(bar, {
    1,
    2,
})

But then your comment made me realize that there are still cases when the first element in the nested table is on the same row as the opening brace and it creates an alignment dilemma, so to speak, even without the outer function call:

-- this looks fine
foo = {1,
       2,
       3}

-- but the following two look strange
foo = {1,
       2,
       3
}

foo = {1,
       2,
       3
      }

-- although less strange than "minus one level of indentation"
foo = {1,
       2,
       3
  }

So I guess it would make sense to introduce a variable for people to "pick their poison" in cases when table elements are aligned, but the close brace is on a separate line.

@edam

edam commented Dec 6, 2019

Copy link
Copy Markdown
Contributor Author

So are you saying to leave it as it is, so the user has the choice? 🤔

@immerrr

immerrr commented May 8, 2020

Copy link
Copy Markdown
Owner

Sorry for the delay here.

I'm going to merge it. Thank you for your work and for your patience!

@immerrr
immerrr merged commit a3a71b1 into immerrr:master May 8, 2020
@immerrr

immerrr commented May 25, 2020

Copy link
Copy Markdown
Owner

Hey!

Looks like lua-indent-nested-block-content-align setting applies not only to nested blocks, but to any alignment in general.

foobar(123,
   456)

In this example there are no nested blocks, but since alignment is disabled, 456 is indented instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants